*: add InstallType to InstallConfig #4209

hexfusion · 2020-09-23T14:24:25Z

As OpenShift 4 extends its use cases additional workflows for how the bootstrapping of the cluster will evolve. For example, assisted-installer uses the openshift-installer to generate manifests and then independently manages the bootstrapping of the cluster after install complete. Because these alternative workflows require in many cases the cluster and operators to observe these changes clear signal is required.

This PR introduces an optional field to the InstallConfig InstallType. This type which will be embedded into the cluster-config-v1 ConfigMap in the kube-system namespace. I feel this change requires very little overhead for maintenance meanwhile providing very important signal to the cluster.

Other uses for this flag could include for example CRC single node cluster [1] and new future edge installations. This allows the logic for how these installs are managed to be handled by the cluster operators instead of additional knobs to the installer itself.

[1] openshift/enhancements#302

enhancement: openshift/enhancements#480
blocking 4.7 epic: https://issues.redhat.com/browse/ETCD-94

openshift-ci-robot · 2020-09-23T14:24:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign abhinavdahiya
You can assign the PR to them by writing /assign @abhinavdahiya in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Sam Batschelet <[email protected]>

hexfusion · 2020-09-23T17:53:14Z

cc @abhinavdahiya @deads2k @ironcladlou @crawford @retroflexer @romfreiman @hardys

ironcladlou · 2020-09-23T18:02:26Z

pkg/types/installconfig.go

+	// InstallType provides signal to the cluster that the installation will not follow the default installer workflow.
+	//
+	// +optional
+	InstallType string `json:"installType,omitempty"`


I feel like there's a more descriptive name to be found, and it should be an enum at least... seems like it will inevitably become mirror of PlatformType to some degree but with different lifecycle implications.

Is a raw enum going to be enough? (It wasn't for platform type.) What's the relationship between this type and and platform types? Which combinations are supported, and what are the clear semantics and cluster implications for each type and combination of type/platform? etc.

Would a vendor annotation on the installconfig make any sense as an alternative if we aren't able to more carefully specify these things right now?

(see: openshift/enhancements#480 (comment))

Is a raw enum going to be enough? (It wasn't for platform type.) What's the relationship between this type and and platform types? Which combinations are supported, and what are the clear semantics and cluster implications for each type and combination of type/platform? etc.

I agree with you in general but the idea is that the logic and relationships that each operator would conclude should be based on the value. We could certainly make this more strict and only define explicitly supported types. Then extended the list as necessary.

Would a vendor annotation on the installconfig make any sense as an alternative if we aren't able to more carefully specify these things right now?

If no agreement can be made I am open to any reasonable signal.

(see: openshift/enhancements#480 (comment))

I agree that tying the install type to a platform doesn't make sense. The idea is to completely platform agnostic as the install type is not related to the platform it is performed on the platform. That was my intention in making the key at root level.

I feel like there's a more descriptive name to be found, and it should be an enum at least

If you have a better name I am open to change.

I'm most interested in clarity of semantics. Possible values, what they mean on their own, what they mean with respect to platform types.

For example, based on what I understand so far, here's a try:

// AssistedInstaller indicates the platform will assume an installation // topology consisting of exactly 3 nodes which will be the only available // nodes to complete cluster bootstrapping. // // The platform may relax certain high availability constraints to achieve // bootstrapping under the constrained topology. // // This install type is only supported when the platform type is BareMetal. type AssistedInstaller InstallType = "AssistedInstaller"

Maybe it needs to be even more specific in terms of the machine pools. Isn't the requirement for there to be a single master pool with 3 replicas and an empty worker pool? This should be unambiguous.

why? we let the user choose how many workers he adds - among all the discovered hardware, we auto-select 3 masters and all the rest become workers

Good point. Is the absence of the dedicated bootstrap node the substantive difference? Again, we should figure out how to define it

~~Worker pools are not assumed but unlike compact clusters workers are possible. @romfreiman keep me honest here.~~

romfreiman · 2020-09-23T18:23:02Z

cc: @eranco74 @yevgeny-shnaidman @ronniel1

openshift-ci-robot · 2020-09-23T19:48:17Z

@hexfusion: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-ovirt	`a89f586`	link	`/test e2e-ovirt`
ci/prow/e2e-libvirt	`a89f586`	link	`/test e2e-libvirt`
ci/prow/e2e-metal-ipi	`a89f586`	link	`/test e2e-metal-ipi`
ci/prow/e2e-crc	`a89f586`	link	`/test e2e-crc`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

hardys · 2020-09-29T09:10:00Z

I'm looking at the ClusterProfile proposal and wondering if we have some overlapping requirements here - that's focussed on the CVO, but perhaps we should have a "2 master bootstrap" cluster profile that sets some data via a manifest, that the CEO can consume to influence the bootstrap behavior?

Also in the initial description it's mentioned that this requirement could include CRC/edge single node requirements, and we can't necessarily say all of those environments will be deployed via the assisted installer (could be UPI without the assisted workflow if customers are already using that approach and have their own automation in place, for example) so relying on a flag that defines the InstallType may prove inflexible in those cases?

hardys · 2020-09-29T09:19:50Z

Also see #3986

hexfusion · 2020-09-29T13:15:14Z

I'm looking at the ClusterProfile proposal and wondering if we have some overlapping requirements here - that's focussed on the CVO, but perhaps we should have a "2 master bootstrap" cluster profile that sets some data via a manifest, that the CEO can consume to influence the bootstrap behavior?

The point of InstallType is that we are completely explicit about the intent of install to the cluster. To me "2 master bootstrap" is not clear it is a generalization. Where "AssistedInstaller" is very clear and explicit. How the signal is passed really is not so important to the cluster as what the signal means. The cluster needs to understand with certainty if the cluster will not look like it normally should why, and what it should expect.

Also in the initial description it's mentioned that this requirement could include CRC/edge single node requirements, and we can't necessarily say all of those environments will be deployed via the assisted installer (could be UPI without the assisted workflow if customers are already using that approach and have their own automation in place, for example) so relying on a flag that defines the InstallType may prove inflexible in those cases?

So what I mean is that the InstallType can include those install types once supported. As the key is root level it is not bound to any provider etc. This would encompass only installs that have a workflow that is nonstandard or on its own would be considered unsupported. So back to the "2 master bootstrap" this would require some feature of supporting 2 masters so that the shortcomings of 2 member clusters were resolved in some smart way. The feature would be defined as an InstallType.

To be clear I don't have super strong feeling on how the signal is passed but it needs to be clear.

type InstallType string

const (
  // AssistedInstaller what cluster shoudl expect is....
  type AssistedInstaller InstallType = "AssistedInstaller"
  type CodeReadyContainers InstallType = "CodeReadyContainers"
  type NewEdgeSomething InstallType = "NewEdgeSomething"
)

jstuever · 2020-10-05T21:10:00Z

/cc @abhinavdahiya

gbraad · 2020-10-13T06:52:20Z

I'm looking at the ClusterProfile proposal and wondering if we have some overlapping requirements here - that's focussed on the CVO, but perhaps we should have a "2 master bootstrap" cluster profile that sets some data via a manifest, that the CEO can consume to influence the bootstrap behavior?

please find us on #cluster-profiles for further discussion ?

Also, as I understood, an InstallType that is specifc to "CodeReady Containers", etc is not actually wished for. The profiles should handle being able to handle the needed functionality for specific use-cases, like NonHA, etc

hexfusion · 2020-11-07T11:51:58Z

@gbraad I joined your forum but had no response to a few questions I will schedule a meeting next week.

romfreiman · 2020-11-08T07:49:39Z

@hexfusion @gbraad please add me as well

gbraad · 2020-11-09T02:47:21Z

As far as I can see @hexfusion didn't respond on the internal at-reply since the 13th of Oct? Anyway, both @guillaumerose and @cfergeau are working on this. If you have any requests or questions, please schedule a meeting with them.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2020

openshift-ci-robot requested review from jhixson74 and jstuever September 23, 2020 14:24

hexfusion force-pushed the add-installType branch 2 times, most recently from 70f252b to b06a994 Compare September 23, 2020 16:20

*: add InstallType to InstallConfig

a89f586

Signed-off-by: Sam Batschelet <[email protected]>

hexfusion force-pushed the add-installType branch from b06a994 to a89f586 Compare September 23, 2020 17:39

hexfusion changed the title ~~[wip] *: add InstallType to InstallConfig~~ *: add InstallType to InstallConfig Sep 23, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2020

ironcladlou reviewed Sep 23, 2020

View reviewed changes

openshift-ci-robot requested a review from abhinavdahiya October 5, 2020 21:10

hexfusion mentioned this pull request Oct 12, 2020

Add single-node-developer Cluster Profile openshift/enhancements#482

Merged

30 tasks

hexfusion closed this Nov 11, 2020

hexfusion deleted the add-installType branch November 11, 2020 21:51

*: add InstallType to InstallConfig #4209

*: add InstallType to InstallConfig #4209

Uh oh!

Conversation

hexfusion commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 23, 2020

Uh oh!

hexfusion commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ironcladlou Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hexfusion Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romfreiman commented Sep 23, 2020

Uh oh!

openshift-ci-robot commented Sep 23, 2020

Uh oh!

hardys commented Sep 29, 2020

Uh oh!

hardys commented Sep 29, 2020

Uh oh!

hexfusion commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jstuever commented Oct 5, 2020

Uh oh!

gbraad commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hexfusion commented Nov 7, 2020

Uh oh!

romfreiman commented Nov 8, 2020

Uh oh!

gbraad commented Nov 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

hexfusion commented Sep 23, 2020 •

edited

Loading

hexfusion commented Sep 23, 2020 •

edited

Loading

ironcladlou Sep 23, 2020 •

edited

Loading

hexfusion Sep 23, 2020 •

edited

Loading

hexfusion commented Sep 29, 2020 •

edited

Loading

gbraad commented Oct 13, 2020 •

edited

Loading